Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(typings): fixed compiler errors when using @next version of typescript #1700

Merged
merged 1 commit into from
May 25, 2016

Conversation

david-driscoll
Copy link
Member

Description:
This fixes two of the compiler errors that are issues with the vNext version of TypeScript (currently named 1.9, soon to be 2.0 I believe).

The third error is a compiler issue, so I have made no changes to FromEventObservable.

Related issue (if exists):
#1697

@david-driscoll
Copy link
Member Author

As per microsoft/TypeScript#7662 the error is a breaking change to TypeScript 2.0. The fix is to block scope the variable.

constructor(private callbackFunc: Function,
private selector: Function,
private args: any[],
constructor(public callbackFunc: Function,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe we can make dispatch static method in boundcallbackObservable? I would prefer to not expose these values as public.

@rkirov
Copy link

rkirov commented May 24, 2016

Can we merge this change in? We would like to use TS 1.9 in angular2 as soon as possible.

Also, I see another error with @next, microsoft/TypeScript#8795
which will result in broken .d.ts files (but not broken compilation, which can be very confusing).

I recommend tacking on this change:

rewrite for Subject<T>:

  constructor(protected destination?: Observer<T>, protected source?: Observable<T>) {
    super();
  }

to

  constructor(destination?: Observer<T>, source?: Observable<T>) {
    super();
    this.destination = destination;
    this.source = source;
  }

so that it is clear that the arguments are optional but the field declarations are not. Or just make the field declarations in Observable optional too (but that would require all consumers to use 1.9).

@david-driscoll david-driscoll force-pushed the fix/1697 branch 2 times, most recently from 3291d48 to 278ed7b Compare May 25, 2016 01:42
@david-driscoll
Copy link
Member Author

Applied the feedback from @kwonoj and @rkirov that subject thing has bothered me in the past, I guess I never noted that the properties were optional... that's a strange bug for sure.

@kwonoj
Copy link
Member

kwonoj commented May 25, 2016

Is my understanding correct above behavior (mark field optional) will be fixed by ts compiler eventually?

@david-driscoll
Copy link
Member Author

I'm sure it will, but doesn't seem like much harm to make them fields to get it working today.

@kwonoj
Copy link
Member

kwonoj commented May 25, 2016

Those practices are widespread in our codebase not only in thise, so concerned even we apply immediate fix for this change there are still places have broken behavior. Maybe better to track compiler updates to resolve issue completely.

@david-driscoll
Copy link
Member Author

doesn't matter to me, I can flip it back. 😄

@benlesh
Copy link
Member

benlesh commented May 25, 2016

LGTM. :shipit:

@benlesh benlesh merged commit a98bec7 into ReactiveX:master May 25, 2016
@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants